Skip to content

refactor(rust): read the environment through figment, and rename ClientConfig to HttpConfig - #708

Closed
wkirschenmann wants to merge 2 commits into
wk/feat/rust-proxyfrom
wk/refactor/rust-config-figment
Closed

refactor(rust): read the environment through figment, and rename ClientConfig to HttpConfig#708
wkirschenmann wants to merge 2 commits into
wk/feat/rust-proxyfrom
wk/refactor/rust-config-figment

Conversation

@wkirschenmann

Copy link
Copy Markdown
Contributor

First of three PRs replacing #705, split after its base (#696) moved and left it conflicting.
This one: adopt figment for environment reading, then rename ClientConfig(Args) to
HttpConfig(Args) ahead of a GrpcConfig a later PR introduces for tonic/retry settings.

  • Replaces the hand-rolled deserializer (which could not support #[serde(flatten)], needed
    by the next PR's thematic-unit grouping) with figment's Env provider, plus a text/
    secret_text deserialize_with shim so every field still carries plain text regardless of
    figment's scalar coercion.
  • Retires HttpConfig::from_env (the resolved type holds a Uri/CertificateDer/
    PrivateKeyDer, none of which has a Deserialize worth offering). armonik::client::env
    now composes HttpConfigArgs::from_env + HttpConfig::from_config_args in Client::new,
    which returns a new NewClientError instead of ConnectionError - a breaking change, but
    this crate has not been published with either shape yet.

Stacked as: this PR -> wk/refactor/rust-config-units -> wk/feat/rust-pkcs12.

…option as text

The previous serde::Deserializer for ClientConfigArgs::from_env only implemented
deserialize_struct, walking a static field list against std::env::var_os. That cannot
support #[serde(flatten)], which grouping fields into thematic sub-structs later needs while
keeping today's environment variable names.

config and conf were both evaluated and rejected first: both lower-case an environment
variable's key before any case-conversion option runs, which cannot read ArmoniK's inherited
PascalCase convention (GrpcClient__CertPem) no matter how they are configured, confirmed by
reproducing the failure against their vendored source. figment's Env provider, used as
Env::prefixed(prefix).lowercase(false), preserves that casing, verified against its own
source and a scratch project before adopting it.

figment's Env provider parses each variable's raw text into a typed value before serde ever
sees it: a bare 3 becomes an integer, true becomes a boolean, and a value entirely enclosed
in brackets or braces becomes a list or object. Every field of ClientConfigArgs is meant to
carry plain text regardless of source, so a text/secret_text deserialize_with pair, applied
to every field, coerces a scalar back to its own spelling; a value that is itself a full
bracketed list is refused with a message naming its escape hatch, wrapping it in a literal
pair of double quotes.

The three boolean fields become String for the same reason: a deserialize_with shim's
signature never carries the field it was called for, so the vocabulary wider than
true/false is now interpreted once, in from_config_args, which is what can still name the
option on a bad value (a new ConfigError::InvalidBool).

ClientConfig::from_env keeps its old zero-argument signature for now, hardcoding
GrpcClient__ as the prefix: making it a parameter of its own, and retiring the method
entirely in favour of composing HttpConfigArgs::from_env in the armonik crate, is the next
commit's concern.
…tpConfigArgs, and retire ClientConfig::from_env

Ahead of a GrpcConfig a later PR introduces for tonic/retry-specific settings.

HttpConfig::from_env is removed: HttpConfig holds a Uri, a CertificateDer, a PrivateKeyDer,
none of which has a Deserialize worth offering, so a from_env on it implied a capability the
type does not have. Only HttpConfigArgs deserialises; a caller composes the two steps
itself, the way armonik's Client::new now does. ConfigError::Env goes with it: once it
wrapped exactly one thing (EnvFieldError), it was pure indirection.

armonik gains client::env: NewClientError splits Env (reading HttpConfigArgs::from_env),
Config (HttpConfig::from_config_args rejecting what it read) and Connect (with_config
failing to reach the endpoint), and ARMONIK_PREFIX names GrpcClient__ in one place rather
than as a literal at the one call site that used it. Client::new returns NewClientError
instead of ConnectionError: a breaking change, but this crate has not been published with
either shape yet.
@sonarqubecloud

sonarqubecloud Bot commented Aug 6, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines Covered Coverage Threshold Status
1478 1247 84% 0% 🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: d676757 by action🐍

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d676757f5e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +89 to +90
"GrpcClient__Endpoint",
Some("http://localhost:5001"),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Stop mutating the endpoint in the environment-reader test

In the Rust cargo test --workspace --all-features workflow, this test changes the process-global endpoint even though existing unit tests such as client/agent.rs:275 concurrently call Client::new() under named serial groups that do not share this test's default serial lock. If they overlap, the no-TLS job is redirected from its server on port 5000 to port 5001, while TLS jobs can be redirected from HTTPS to HTTP, causing nondeterministic connection failures; this test only asserts UserAgent, so it should leave GrpcClient__Endpoint untouched.

Useful? React with 👍 / 👎.

Comment on lines +296 to +297
fn visit_f64<E>(self, value: f64) -> Result<String, E> {
Ok(value.to_string())

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve numeric-looking environment values verbatim

When Figment coerces a textual environment value such as GrpcClient__ProxyPassword=1.0 into an f64, this converts it back to "1", silently changing the credential before proxy authentication; other noncanonical numeric spellings can likewise lose their original representation. These fields are documented as text and the previous environment reader preserved their exact strings, so the provider should read raw values or otherwise retain the original spelling rather than round-tripping through numeric types.

Useful? React with 👍 / 👎.

@wkirschenmann

Copy link
Copy Markdown
Contributor Author

Superseded by the config restack. The Args removal, the thematic units and the option vocabulary are re-authored on top of the re-cut proxy stack rather than on the pre-recut base this PR targets: the machinery moves into a dedicated src/config_utils/ module landing directly on main, then env reading and the JSON schema on top of it, then the proxy option surface as a single unit.

Closing rather than rebasing: this branch's base no longer exists in a form worth rebasing onto, and the content is carried forward by the re-authored series.

@wkirschenmann
wkirschenmann deleted the wk/refactor/rust-config-figment branch August 7, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant